planner: fix partial index wrongly kept due to shared Range pointer mutation - #70002
planner: fix partial index wrongly kept due to shared Range pointer mutation#70002yy782 wants to merge 4 commits into
Conversation
…ointer UnionRanges builds sortRange objects holding pointers to the original Range. When merging adjacent or overlapping ranges, it mutates the pointed Range directly, corrupting the caller's input because the pointer is shared across all sortRange items. This causes CheckPartialIndexes to incorrectly determine that partial index conditions are implied by query conditions, leading to wrong index pruning. Ref pingcap#69779
…p#69779) Verify that a >= 0 does NOT imply a < 3 for partial index pruning, which is the regression case from the shared pointer bug in UnionRanges.
ti-chi-bot
Bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
contribution
|
Hi @yy782. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesRange isolation and planner validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
There was a problem hiding this comment.
⚠️ Not ready to approve
The UnionRanges merge path still assigns HighVal by reference (slice aliasing), which can reintroduce shared-state behavior and undermine the intended isolation fix.
Pull request overview
This PR addresses a planner correctness bug where partial indexes could be incorrectly accepted/pruned due to UnionRanges mutating shared *Range inputs, leading to wrong implication checks and potentially incorrect query results (ref #69779).
Changes:
- Prevents
UnionRangesfrom mutating caller-owned*Rangeinstances by cloning the active merge range. - Adds an integration test case that reproduces and guards against the “partial index chosen when predicate doesn’t imply index predicate” wrong-result scenario.
- Updates the corresponding integration test result file.
File summaries
| File | Description |
|---|---|
| pkg/util/ranger/ranger.go | Clones the working range in UnionRanges to avoid mutating shared *Range pointers during merges. |
| tests/integrationtest/t/planner/core/casetest/index/partialindex.test | Adds a regression query case ensuring a >= 0 does not incorrectly allow using a partial index defined with a < 3. |
| tests/integrationtest/r/planner/core/casetest/index/partialindex.result | Records expected results for the new regression case (ensuring full rowset is returned). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lastRange.encodedStart = objects[i].encodedStart | ||
| lastRange.encodedEnd = objects[i].encodedEnd | ||
| lastRange.originalValue = objects[i].originalValue.Clone() |
There was a problem hiding this comment.
After Clone(), lastRange.originalValue is already an independent Range object. The HighVal assignment on line 694 only changes where this cloned object's HighVal field points — it does not mutate the caller's original Range in any way.
While the cloned HighVal and objects[i].HighVal do share the same underlying array, the returned ranges' HighVal is only ever read (for range endpoint comparison), never mutated in-place by downstream consumers. So this slice aliasing is safe in practice.
I'm leaning toward keeping the current approach — it avoids an unnecessary copy and the semantics are correct for this use case. That said, if you feel a deep copy here adds more confidence, I'm open to it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #70002 +/- ##
================================================
- Coverage 76.3239% 73.5857% -2.7382%
================================================
Files 2041 2060 +19
Lines 559772 583148 +23376
================================================
+ Hits 427240 429114 +1874
- Misses 131631 153479 +21848
+ Partials 901 555 -346
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
/cc @D3Hunter, @hawkingrei |
|
I verified the fix end-to-end: the repro from #69779 returns wrong results (3 rows instead of 5) on master, and passes with your patch. Your root-cause diagnosis is correct: UnionRanges mutates the caller's *Range in place, so implCompareExpr ends up comparing the pre-predicate range against itself (the ran == other fast path in Range.Equal) and always accepts the implication when a merge occurs. A few requested changes:
|
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
UnionRanges reused the input slice's backing array and modified Range objects in-place during merging, causing partial index pruning to retain an unsafe access path and return wrong results. Fix by using a copy-on-write strategy: clone a Range only when a merge actually modifies it. Updated the doc comment and added a unit test that verifies the non-mutation contract. Close pingcap#69779
ti-chi-bot
Bot
added
size/L
yy782
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/util/ranger/ranger.go (1)
691-698: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClone only when the merged endpoint changes.
Line 691 clones for contained/duplicate ranges even though lines 695-698 make no mutation. Move the copy-on-write block inside the endpoint-extension branch to preserve the requested allocation avoidance for large union workloads.
Proposed fix
- if !detached { - lastRange.originalValue = lastRange.originalValue.Clone() - detached = true - } if bytes.Compare(lastRange.encodedEnd, objects[i].encodedEnd) < 0 { + if !detached { + lastRange.originalValue = lastRange.originalValue.Clone() + detached = true + } lastRange.encodedEnd = objects[i].encodedEnd lastRange.originalValue.HighVal = objects[i].originalValue.HighVal🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/ranger/ranger.go` around lines 691 - 698, In the range-merging logic, move the copy-on-write block that clones lastRange.originalValue inside the encodedEnd extension condition. Ensure cloning and setting detached occur only when bytes.Compare(lastRange.encodedEnd, objects[i].encodedEnd) < 0 and the merged endpoint fields are updated, while contained or duplicate ranges avoid allocation.pkg/util/ranger/ranger_test.go (1)
2690-2709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a second merge group after detachment resets.
This case ends immediately after the non-merge transition, so removing line 703 would still pass. Add a disjoint overlapping pair (for example
[6,8],[7,9]) and assert both groups leave their original ranges unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/ranger/ranger_test.go` around lines 2690 - 2709, Extend the UnionRanges test around the existing r1/r2/r3 setup with a second disjoint overlapping pair, such as [6,8] and [7,9], so merging continues after the detachment reset. Assert the merged output contains both groups and verify every original range in both groups retains its bounds and exclusion flags unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/util/ranger/ranger_test.go`:
- Around line 2690-2709: Extend the UnionRanges test around the existing
r1/r2/r3 setup with a second disjoint overlapping pair, such as [6,8] and [7,9],
so merging continues after the detachment reset. Assert the merged output
contains both groups and verify every original range in both groups retains its
bounds and exclusion flags unchanged.
In `@pkg/util/ranger/ranger.go`:
- Around line 691-698: In the range-merging logic, move the copy-on-write block
that clones lastRange.originalValue inside the encodedEnd extension condition.
Ensure cloning and setting detached occur only when
bytes.Compare(lastRange.encodedEnd, objects[i].encodedEnd) < 0 and the merged
endpoint fields are updated, while contained or duplicate ranges avoid
allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1703511-6cf1-43d0-a6c3-bf4692b9f891
📒 Files selected for processing (4)
pkg/util/ranger/ranger.gopkg/util/ranger/ranger_test.gotests/integrationtest/r/planner/core/casetest/index/partialindex.resulttests/integrationtest/t/planner/core/casetest/index/partialindex.test
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integrationtest/t/planner/core/casetest/index/partialindex.test
- tests/integrationtest/r/planner/core/casetest/index/partialindex.result
Only clone a Range when the merge truly extends it (encodedEnd < newEnd), avoiding unnecessary cloning when merging identical ranges. Add a second overlapping group to the unit test to cover the "end a merge group, start a new one" code path. Close pingcap#69779
|
A subtle point: UnionRanges reuses the backing array of the input slice (using ranges[:0] + append), meaning the caller's slice elements will be overwritten with new pointers. The only guarantee is that the original Range objects (r1, r2, r3) are not mutated. The doc comment currently says "does not modify the caller's ranges" — I wonder if that's sufficiently clear, or should we add an inline comment explaining the slice reuse? Alternatively, are there other options we should consider? |
What problem does this PR solve?
Issue Number: close #69779
Problem Summary:
What changed and how does it work?
When query condition is a >= 0 and partial index condition is a < 3, CheckPartialIndexes wrongly keeps/retains the index (instead of pruning it), leading to incorrect query results. The root cause is UnionRanges mutating shared Range pointers, corrupting the caller's input and causing the implication check to fail. See issue #69779 for details.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit